-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
multiregionccl: add a cold start latency test #92826
multiregionccl: add a cold start latency test #92826
Conversation
3085659
to
0f33e97
Compare
This is an oversight from cockroachdb#92588. Testing will come in cockroachdb#92826. Epic: CRDB-18596 Release note: None
972c163
to
d3874fb
Compare
93294: dev: do not log noisy bep flag r=rickystewart a=healthy-pod Release note: None Epic: none 93342: sql: partition lease table when optimizing system database for MR r=ajwerner a=ajwerner This is an oversight from #92588. Testing will come in #92826. Epic: CRDB-18596 Release note: None Co-authored-by: healthy-pod <[email protected]> Co-authored-by: Ahmad Abedalqader <[email protected]> Co-authored-by: Andrew Werner <[email protected]>
d3874fb
to
f70079a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jeffswenson)
Release note: None
Release note: None
Release note: None
f70079a
to
e926308
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
) | ||
localities := makeLocalities(regionLatencies, numNodes, numAZsPerRegion) | ||
regions := make([]string, len(localities)) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is the extra block wrapping l.Find intentional?
t.Log("pre running test to allocate instance IDs") | ||
t.Log("result", localities, runAllTests()) | ||
t.Log("running test with no connectivity from sql pods to remote regions") | ||
blockCrossRegionTenantAccess.Store(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking cross-region network traffic is a neat check.
e926308
to
e3e4bdc
Compare
This commit adds a test which creates an MR serverless cluster and then boots the sql pods in each region while disallowing connectivity to other regions. It also simulates latency to make sure the routing logic works and to provide a somewhat realistic picture of what to expect. Release note: None
e3e4bdc
to
4c59d79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR!
bors r+
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)
pkg/ccl/multiregionccl/cold_start_latency_test.go
line 71 at r8 (raw file):
Previously, JeffSwenson (Jeff Swenson) wrote…
nit: is the extra block wrapping l.Find intentional?
Done.
Build succeeded: |
This commit adds a test which creates an MR serverless cluster and then
boots the sql pods in each region while disallowing connectivity to other
regions. It also simulates latency to make sure the routing logic works
and to provide a somewhat realistic picture of what to expect.
Epic: CRDB-18596
Release note: None